-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: ensure email setting model functions as expected #1136
fix: ensure email setting model functions as expected #1136
Conversation
cb44d5d
to
8c3a596
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1136 +/- ##
==========================================
+ Coverage 87.03% 87.12% +0.08%
==========================================
Files 388 392 +4
Lines 8069 8172 +103
Branches 1944 2017 +73
==========================================
+ Hits 7023 7120 +97
- Misses 997 1002 +5
- Partials 49 50 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is fixing this issue after testing this branch locally; I still have the same issue as described the ticket:
If email settings are already disabled for a course enrollment, the email settings modal does not seem to ever open
1850887
to
bb54275
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this largely resolves the immediate bug, the current changes would introduce another bug since the "Email settings" option is now exposed for assignments (and likely also requested courses, too through the Browse-and-Request feature), which would result in an error being thrown if a user were to submit/confirm their email settings choice in the modal. See below comment for further details.
@@ -341,19 +334,14 @@ const BaseCourseCard = ({ | |||
); | |||
}; | |||
|
|||
const renderEmailSettingsModal = () => { | |||
if (!hasEmailsEnabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with removing hasEmailsEnabled
stuff altogether is that not all course enrollment card types/variants should include the email settings dropdown. For example, assignment cards (AssignedCourseCard
) should not expose the "Emails settings" dropdown given the assignment is not yet a legit course enrollment (i.e., learner hasn't accepted their assignment yet), so the user would likely face an error when trying to submit the email settings modal to save their choice.
As is, removing this logic, incorrectly exposes the "Email settings" dropdown item for assignments:
When keeping this hasEmailsEnabled
logic, it might be falsey for the following reasons:
BaseCourseCard
is of type/variantAssignedCourseCard
, wherehasEmailsEnabled
isundefined
.BaseCourseCard
is not of type/variantAssignedCourseCard
, wherehasEmailsEnabled
represents the state of the whether the learners's current choice of whether emails are enabled for the course enrollment (i.e.,false
,true
).
The modal previously wasn't opening when a learner has emails disabled for an enrollment because of the (2) case above, whereby false
is falsey, fulfilling this condition and thus returning null
.
We would only want to short-circuit the rendering of the EmailSettingsModal
when hasEmailsEnabled
is a legit, defined value (i.e., not undefined
/null
).
For all BaseCourseCard
types/variants, the data is run through the transformCourseEnrollment
function, and sets the hasEmailsEnabled
property based on the emailsEnabled
property for the given course enrollment. If the emailsEnabled
property is undefined
/null
(i.e., when the "course enrollment" here is actually representing an assignment, where email settings is not applicable).
By removing the hasEmailsEnabled
stuff altogether, notably within getDropdownMenuItems
, the "Email settings" dropdown item is incorrectly exposed for AssignedCourseCard
, which would introduce a different bug since the related email settings API would error when called within the modal.
I might suggest keeping all the hasEmailsEnabled
stuff but make sure the BaseCourseCard
component properly handles hasEmailSettings
as undefined
/null
and false
as separate falsey conditions throguhout to mitigate these issues. Consider using the isDefinedAndNotNull
helper function (source), e.g.:
const renderEmailSettingsModal = () => {
if (!isDefinedAndNotNull(hasEmailsEnabled)) {
return null;
}
};
97a4978
to
ca3f765
Compare
ca3f765
to
5e35e1b
Compare
For all changes
Only if submitting a visual change